Skip to content

Change ConcurrenyMIddleware service extensions method #19324

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

Kahbazi
Copy link
Member

@Kahbazi Kahbazi commented Feb 25, 2020

Change ConcurrenyMIddleware service extensions method based on #12453 (comment)

@Tratcher
Copy link
Member

Breaking change procedures require keeping the old extensions and adding new ones.

@analogrelay
Copy link
Contributor

As @Tratcher said, this is an unfortunate situation since it really shouldn't have been done this way but now that it was, it's a breaking change to fix it :). I expect what we would prefer to do is this:

  1. Add a services.AddConcurrencyLimiter() extension that returns a ConcurrencyLimiterBuilder (like we do with other things like SignalR:
    var builder = new SignalRServerBuilder(services);
    )
  2. Hang the AddQueuePolicy and AddStackPolicy extension methods off that instead.

But we'd also need to leave the existing methods in place for 5.0 at least (marked as obsolete) and then consider removing them later.

@analogrelay analogrelay added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Mar 2, 2020
@Kahbazi
Copy link
Member Author

Kahbazi commented Apr 7, 2020

There is an open issue (#19823) on adding more feature to Concurency Limiter. Should I close this PR to prevent future breaking changes since the naming can be handled in the issue?

@halter73
Copy link
Member

halter73 commented Aug 3, 2020

There is an open issue (#19823) on adding more feature to Concurency Limiter. Should I close this PR to prevent future breaking changes since the naming can be handled in the issue?

@Kahbazi Sorry for the slow feedback on this, but the short answer is yes, we've decided to close this.

This PR clearly improves on what we currently have in our opinion, but we think we are going to want to make further configuration changes to support endpoint-based concurrency limiting (#20835) with named policies similar to how CorsOptions works today. It seems unlikely that endpoint-based concurrency will make it in time for .NET 5, but even so we don't want to make a breaking change in one major release just to break the same API in the next. I plan to file an issue with the new proposed design soon.

@halter73 halter73 closed this Aug 3, 2020
@halter73
Copy link
Member

halter73 commented Aug 3, 2020

The design we want is very similar to what you outlined in #19823 (comment). I'm looking at that as the starting point for the new proposal. I'll probably just respond to that issue.

@amcasey amcasey added the area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlewares label Jun 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlewares
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants